Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added direct upload from video camera #12995

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

timedin-de
Copy link
Contributor

@timedin-de timedin-de commented May 9, 2024

Regarding feature request #7358

  • Adding Option to directly upload a video

  • If camera permissions are not already granted, it will ask if for video or image after granting (its seems not not to be possible to get the camera-action after the permission request)

  • It seems that the videointent doesn't work on emulator?

  • Tested successfull with a real GooglePixel 6a

  • Tests written, or not not needed

@timedin-de timedin-de changed the title Added upload from video camera #7358 Added upload from video camera May 9, 2024
@timedin-de timedin-de changed the title Added upload from video camera Added direct upload from video camera May 9, 2024
Copy link
Collaborator

@alperozturk96 alperozturk96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution.

I try to upload video directly on Android 14 emulator, after completing video shooting upload didn't work. The video file didn't upload.

After my second attempt, the app is crashed. Logcat didn't show any crash log for nextcloud package and general android

Could you test via Android 14 emulator and share your findings with us?

@timedin-de
Copy link
Contributor Author

timedin-de commented May 14, 2024

Thanks for the contribution.

I try to upload video directly on Android 14 emulator, after completing video shooting upload didn't work. The video file didn't upload.

After my second attempt, the app is crashed. Logcat didn't show any crash log for nextcloud package and general android

Could you test via Android 14 emulator and share your findings with us?

Thanks for testing, thats what i meant with "It seems that the videointent doesn't work on emulator?", I tried countless times using the videointent on an emulator and it crashes or exits. I currently have no idea why this happens. On a hardware phone (Pixel 6a) it just works... (i was unsure if it was maybe just my underpowered laptop, but this ensures the bug)

I'll try some tests with only video intents (outside of the nextcloup app) on emulator to check for the issue.

@timedin-de
Copy link
Contributor Author

Hey, I'm sorry, but I currently don't have the time (and motivation) to further check on this issue in the emulator. Maybe someone else finds the problem, or just discard the PR?

@alperozturk96
Copy link
Collaborator

Hey, I'm sorry, but I currently don't have the time (and motivation) to further check on this issue in the emulator. Maybe someone else finds the problem, or just discard the PR?

I'll check it. Thank you for your contribution; it's wonderful to see support from the community 👍

@JonasMayerDev
Copy link
Collaborator

For me, the PR seems to work on real device (One Plus running Android 12).

@JonasMayerDev
Copy link
Collaborator

JonasMayerDev commented May 21, 2024

What do you think of not having 2 different "upload from camera" in the Dialog Fragment but just one and then showing the user the dialog that's currently shown when accepting the permissions with the options video and image? I think the DialogFragment is already a little crowded, and it would make the experience a little more consistent since the dialog is currently only shown once regardless if "upload from camera" or "upload from video camera" is clicked...

@alperozturk96 @timedin-de What do you think?

@timedin-de
Copy link
Contributor Author

@timedin-de What do you think?

Yep I could do it this way if you agree on it.
It would mean one more click to open the camera, but i think would cause less confusion, (especially the current additional prompt after the permission granting).

I chose the current approach because of the comment in the issue
by @joshtrichards in #7358 (comment)

@JonasMayerDev
Copy link
Collaborator

@timedin-de thank you! I guess the best persons to decide on that is the design team @nextcloud/designers
To summarize for them:

Option 1 for adding a "capture video" implementation would look something like this:

image

Pro: One click video capture
Con: Too crowded dialog when clicking plus button, Inconsistent since dialog from option 2 would still show once when accepting video permissions.

Option 2 would look something like this:

Screenshot_2024-05-21-15-59-40-98_06ea656a9ba653ff96746f477f32d0f9

Pro: Consistent, less crowded
Con: 2 Clicks for video capture

@szaimen
Copy link
Contributor

szaimen commented May 22, 2024

Honestly I dont unterstand what is the difference between upload from camera and upload from video camera?

@timedin-de
Copy link
Contributor Author

Honestly I dont unterstand what is the difference between upload from camera and upload from video camera?

On Android when a camera gets started it needs to be specified if it should record a video or take a picture (you cant decide/change it inside of the camera preview).

So "video camera" starts in video-recording-mode, "camera" in photo-mode

@szaimen
Copy link
Contributor

szaimen commented May 22, 2024

I see. Then maybe rename the entries to Take a video and upload and Take a photo and upload ?

@JonasMayerDev
Copy link
Collaborator

JonasMayerDev commented May 22, 2024

@szaimen Yes good point. But the question is if we should even use 2 separate buttons (video and image) when clicking on the plus in the app. Or if we combine it in one button and the ask the user in a dialog if he wants video or image (prefered option for me and Alper). Take a look at my message for the pro and cons in my opinion: #12995 (comment)

Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@szaimen
Copy link
Contributor

szaimen commented May 28, 2024

I would probably indeed combine these two things into one option:
Take photo or video and upload and keep using the camera icon.

Then if clicking on that, indeed open a popup and ask this question but put the photo button to the right as it is more likely that you want to take a photo imho.

@timedin-de
Copy link
Contributor Author

timedin-de commented May 28, 2024

I would probably indeed combine these two things into one option:
Take photo or video and upload

So change the text for the file-action, instead of leaving it "Upload from camera"?

@JonasMayerDev
Copy link
Collaborator

@timedin-de @szaimen I would leave it as it was. "Take photo or video and upload" feels a little long, an "Upload from camera" is already expressive enough in my opinion?

@szaimen
Copy link
Contributor

szaimen commented May 28, 2024

@timedin-de @szaimen I would leave it as it was. "Take photo or video and upload" feels a little long, an "Upload from camera" is already expressive enough in my opinion?

I think "Take photo or video and upload" is better but if it is too long "Upload from camera" should work as well

@timedin-de timedin-de reopened this May 28, 2024
@timedin-de
Copy link
Contributor Author

Ok I left it on "Upload from camera".

For implementation I moved the permissons-code from FileOperationsHelper to OCFileListFragment, I thought the dialog should be part of the Fragment not the FileHelper. Hit me up if I should change it.

@@ -443,7 +444,7 @@ public void onRequestPermissionsResult(int requestCode, @NonNull String[] permis
// If request is cancelled, result arrays are empty.
if (grantResults.length > 0 && grantResults[0] == PackageManager.PERMISSION_GRANTED) {
// permission was granted
getFileOperationsHelper().uploadFromCamera(this, FileDisplayActivity.REQUEST_CODE__UPLOAD_FROM_CAMERA);
getOCFileListFragmentFromFile().directCameraUpload();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

displaying the dialog in OCFileListFragment


if (!file.renameTo(renamedFile)) {
DisplayUtils.showSnackMessage(getActivity(), "Fail to upload taken image!");
DisplayUtils.showSnackMessage(getActivity(), R.string.error_uploading_direct_camera_upload);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not regarding the video-upload itself, small improvement

Comment on lines +79 to +84
viewThemeUtils.platform.colorImageView(binding.menuIconUploadFiles, ColorRole.PRIMARY);
viewThemeUtils.platform.colorImageView(binding.menuIconUploadFromApp, ColorRole.PRIMARY);
viewThemeUtils.platform.colorImageView(binding.menuIconDirectCameraUpload, ColorRole.PRIMARY);
viewThemeUtils.platform.colorImageView(binding.menuIconScanDocUpload, ColorRole.PRIMARY);
viewThemeUtils.platform.colorImageView(binding.menuIconMkdir, ColorRole.PRIMARY);
viewThemeUtils.platform.colorImageView(binding.menuIconAddFolderInfo, ColorRole.PRIMARY);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not regarding video upload. Fixing Deprecation

Copy link
Collaborator

@JonasMayerDev JonasMayerDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me on OnePlus Nord 2 and code looks good to me.

@alperozturk96
Copy link
Collaborator

alperozturk96 commented Jun 18, 2024

Thanks for the PR and contribution. Amazing work 💯

I just did some small refactoring and add FIXME note for uploading video from emulator. I can't push commit for this branch because it's not branched out from our master branch.

Could you please apply this git patch? Then I can approve it and hopefully we can merge it 🙂

refactor.patch

@timedin-de
Copy link
Contributor Author

timedin-de commented Jun 18, 2024

@alperozturk96 Thank you for your work and time.

Until now i haven't worked with patches, when i just apply it I get an error. Should I just:

  • [1] undo my commit, apply the patch (with git apply refactor.patch or git am < refactor.patch and force-push or
  • [2] apply the patch on another branch (without my commit) and create a merge-commit to the fork-master branch with your refactors
  • [3] directly apply the patch to the current upstream master branch

And how do I have to handle signing off etc.

@JonasMayerDev
Copy link
Collaborator

@timedin-de not 100% sure but I would suggest number 1 ("remove your commits and add them via the patch back") since the patch seems to contain all changes you made compared to master. You can sign your commit with -S so eg. git commit -S -m "message" after you applied the patch

Signed-off-by: TimedIn <git@timedin.net>
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.30.0 milestone Jun 19, 2024
@AndyScherzinger AndyScherzinger merged commit 5c6ddae into nextcloud:master Jun 19, 2024
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants